Fix Gen.pick producing only a subset of possible combinations #1119
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the Gen.pick method where incorrect operator precedence caused the random insertion index calculation to produce a biased distribution, resulting in only a subset of possible combinations being generated.
- Fixes operator precedence in random index calculation by adding parentheses around
(x & Long.MaxValue) - Adds a test to verify that
Gen.pickproduces the expected number of distinct combinations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| core/shared/src/main/scala/org/scalacheck/Gen.scala | Fixes operator precedence bug in random index calculation |
| core/jvm/src/test/scala/org/scalacheck/GenSpecification.scala | Adds test to verify correct number of distinct combinations are generated |
|
|
||
| val genPick = pick(n, 1 to nElements) | ||
| // not interested in different permutations, only in distinct combinations | ||
| .map(_.toList.sorted) |
There was a problem hiding this comment.
Out of curiosity, what is the reason for converting the output of pick to List before sorting it? It doesn't seem it is supposed to improve performance or affects the test in any other way.
There was a problem hiding this comment.
It looks like a leftover from my attempts at writing this test. Removed it.
I think, at some point I had sets and also tried to collect the stats on those. It went into .toString on sets which, amusingly, may be different for equal small sets (up to 4 elements). Thus, it ended up with aSet.toList.sorted and later got scrapped, but toList somehow survived 😅
| // that every possible combination appears at least once. It depends on | ||
| // nSamples, but there is no specific math behind it. Rather, it's just an | ||
| // empirically chosen value, that has yielded no failures over thousands of | ||
| // test cycles. |
There was a problem hiding this comment.
…over thousands of test cycles.
Specifically this
+ Gen.pick produces enough distinct combinations: OK, passed 10000000 tests
|
I believe it is good to go, thanks! |
When calculating a random insertion index for an element
Gen.pickuses wrong order of operationsAs written, it first gets the modulo
MaxValue & countand then uses it to mask a randomxto get a positive value. Instead it should mask thexfirst, and only then get a modulo of it.This PR fixes the order of operations.